-
Notifications
You must be signed in to change notification settings - Fork 896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add orchestration stacks to RBAC #18308
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Hope this helps a little
@@ -110,6 +110,7 @@ class Filterer | |||
'MiqRequest' => :descendant_ids, | |||
'MiqRequestTask' => nil, # tenant only | |||
'MiqTemplate' => :ancestor_ids, | |||
'OrchestrationStack' => nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems templates / generic objects tend to be ancestor_ids
,
and core objects tend to be descendant_ids
.
I expected this one to be descendant_ids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, can we remove this?
a. I think this array is only for tenancy and not cloud tenancy
b. Having a nil
in here will not do anything
Also, can you verify that we are not calling into accessible_tenant_ids_strategy
for OrchestrationStack
?
I'm pretty sure we don't need that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me make clear those things...
a. I think this array is only for tenancy and not cloud tenancy
It works togethers. We need find ids from tenants and then find their equivalents in cloud tenants.
in filterer:
-
it goes to scope_to_cloud_tenant
-
then to the TenancyCommonMixin#tenant_id_clause
-
and then in TenancyCommonMixin#tenant_id_clause it calls tenant_id_clause_format but from CloudTenancyMixin
b. Having a
nil
in here will not do anything
so form previous answer (2) it is needed to havenil
here - it means "current" strategy (it is considering just tenant from logged user - no ancestors or descendants)
Also, can you verify that we are not calling into accessible_tenant_ids_strategy for OrchestrationStack?
I'm pretty sure we don't need that
Yes I verified it when I was looking for previous answers. accessible_tenant_ids_strategy
is called also from OrchestrationStack
and it is called here
@kbrock does it make sense now ? If you other questions let me know!
thanks!
8564952
to
c5b3fc1
Compare
@alexander-demichev can you fix the failing tests? |
c5b3fc1
to
8c9fed4
Compare
8c9fed4
to
ac0bae4
Compare
@lpichler can you review this one, thanks |
@Loicavenel can you verify that a user should/should not be able to see the orchestration stacks for a parent or a child? This is the only model that defines the privileges as The |
I am reviewing ... |
spec/lib/rbac/filterer_spec.rb
Outdated
let(:c_t_flavor_other) { FactoryBot.create(:cloud_tenant_flavor, :cloud_tenant => cloud_tenant_other, :flavor => flavor_other) } | ||
let!(:all_objects) { [project1_volume, project2_volume, volume_other, cloud_tenant_other, project1_c_t_flavor, project2_c_t_flavor, c_t_flavor_other] } | ||
let!(:all_objects) { [project1_volume, project2_volume, volume_other, cloud_tenant_other, project1_c_t_flavor, project2_c_t_flavor, c_t_flavor_other, project1_orchestration_stack, project2_orchestration_stack, orchestration_stack_other] } | ||
|
||
it "lists its own project's objects and other objects where tenant_mapping is not enabled" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexander-demichev "....is enabled " ... ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that change, please also update this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aufi @alexander-demichev can you change it please ?
spec/lib/rbac/filterer_spec.rb
Outdated
@@ -2540,6 +2552,15 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) | |||
|
|||
results = described_class.search(:class => Flavor, :user => other_user).first | |||
expect(results).to match_array [project1_flavor, project2_flavor, flavor_other] | |||
|
|||
results = described_class.search(:class => OrchestrationStack, :user => project1_user).first | |||
expect(results).to match_array [project1_orchestration_stack, project2_orchestration_stack, orchestration_stack_other] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we are querying all OrchestrationStacks so here can be
OrchestrationStack.all
instead of [project1_orchestration_stack, project2_orchestration_stack, orchestration_stack_other]
I guess it is more visible that it is not affecting anything if OpenStack mapping is disabled
strategy is nil, it means Orch.Stacks from current tenant. Those case comes to my mind: questions:
@alexander-demichev |
ac0bae4
to
bebf685
Compare
I feel that when tenant mapping is disabled, we should still respect some form of tenancy. But right now, the tests are saying that the flag means "skip all tenancy". @agrare Who is the business person who can define |
@gtanzillo knows more about tenancy than I do, hopefully he can help 🤞 |
We did implement tenancy in few steps, first it was Instances only and then network and storage.. Stack was most likely left over... This is when using Tenant Mapping between our tenancy and OpensTack tenancy, correct? |
let me check, it but if tenant mapping is disabled we don't have a way how to do "tenancy" and basically tenant mapping is way how to do tenancy for "cloud" models(Stacks, CloudSubnet,... ).
yes exactly, nowadays it is only for OpenStack tenancy |
@lpichler one day, we should list all Objects that can be Tenant related. |
@lpichler Hi Libor, is there something needed to be updated before merging? (Alex is on PTO, so asking there, thanks) |
Yes, otherwise there is no filtering and CF user can see the same inventory as OpenStack admin user entered during adding OSP provider credentials. Tenant mapping allows filtering inventory for Users synced from OSP to CF based on its project/tenant (in CF modelled as CloudTenant; that can be linked to a CF Tenant which is used for filtering)
If UT1 is not created by Tenant Mapping feature, there is no change/no filtering. Otherwise it depends on CloudTenant which is linked to UT1 and User used to login to CF.
All yes - see only objects within own tenant. An access inheritance is off by default in OpenStack (it can be set, but that would bring quite lot of complexity IMO)
Feel free correct me! |
Does I'm having trouble having a model that supports (non cloud) tenancy with an option that says ignore tenancy. |
@aufi Thank you for your response and reviewed it again it looks good to me, I am considering that all cases are covered as I mentioned in comment @kbrock Did you mean other model than
I am not sure I understand this concern correctly - it is not about option with saying "ignore tenancy" Those changes are adding tenancy control for cloud object: We started this effort in #14036 and this PR just extends it about Anyway as this is related to RBAC any concern is very welcomed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@miq-bot assign @gtanzillo or @kbrock :) |
@lpichler 'gtanzillo or kbrock :)' is an invalid assignee, ignoring... |
@lpichler ugh. yea, does |
Aah, you are right:
|
I'm good to merge after those 2 proposed changes |
bebf685
to
0719af3
Compare
Checked commit alexander-demicev@0719af3 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
Thanks 👍 |
@aufi thank you. sorry for all the red tape |
Add orchestration stacks to RBAC
https://bugzilla.redhat.com/show_bug.cgi?id=1597125
@miq-bot assign lpichler